Skip to content

Conversation

@bsmartradio
Copy link
Contributor

No description provided.

Copy link
Member

@kfindeisen kfindeisen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The most glaring issue is the code duplication with lsst/ip_diffim#418, but I also have one big-picture question: what, exactly, is this code for? It doesn't seem to use the Sattle results for anything. Can it all just be removed?

@ebellm
Copy link
Contributor

ebellm commented Jul 16, 2025

@kfindeisen The big picture is that sattle needs the exact exposure time and the boresight ra & dec to populate a cache that will later be used in ip_diffim.detectAndMeasure. That cache takes a small amount of time to populate, so we want to call it early in the pipeline execution process so we're not waiting on it later during detectAndMeasure. This call (to /visit_cache) doesn't return any data to pipelines.

(We have taken steps on the service side to make the service idempotent and able to ignore 189 duplicate requests.)

@kfindeisen
Copy link
Member

kfindeisen commented Jul 16, 2025

The big picture is that sattle needs the exact exposure time and the boresight ra & dec to populate a cache that will later be used in ip_diffim.detectAndMeasure. That cache takes a small amount of time to populate, so we want to call it early in the pipeline execution process so we're not waiting on it later during detectAndMeasure.

I think a comment to that effect would be helpful (though, again, factoring the code might make that unnecessary if the new code is self-descriptive enough).

@ebellm ebellm force-pushed the tickets/DM-50987 branch from b4bc105 to 6db6804 Compare July 20, 2025 14:59
Copy link
Member

@kfindeisen kfindeisen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. As before, many of my comments from lsst/ip_diffim#418 apply here as well.

@ebellm ebellm force-pushed the tickets/DM-50987 branch from 6db6804 to c989a62 Compare July 23, 2025 05:13
@ebellm ebellm merged commit 2a8a10c into main Jul 23, 2025
4 of 5 checks passed
@ebellm ebellm deleted the tickets/DM-50987 branch July 23, 2025 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants